#2232: Accessible markup and unit test coverage for admin lists #3290
Conversation
rachidatecs
requested review from
a team,
Katherine-Osos,
lizpearl,
therealslimhsiehdy,
Matt-Spence,
CocoByte,
dave-kennedy-ecs,
erinysong,
asaki222 and
zandercymatics
|
🥳 Successfully deployed to developer sandbox el. |
|
🥳 Successfully deployed to developer sandbox el. |
|
@rachidatecs This PR looks pretty code heavy. What are you hoping that designers review? |
|
🥳 Successfully deployed to developer sandbox el. |
Accessibility readout on the domain request page for list-like content: alternative domains and websites |
|
🥳 Successfully deployed to developer sandbox el. |
| {% with total_websites=field.contents|split:", " %} | ||
| {% if total_websites|length == 1 %} | ||
| <p class="margin-y-0 padding-y-0"> | ||
| <a href="{{ total_websites.0 }}" target="_blank"> | ||
| {{ total_websites.0 }} | ||
| </a> | ||
| </p> | ||
| {% elif total_websites|length > 1 %} | ||
| <ul class="margin-top-0 margin-left-0 padding-left-0{% if total_websites|length > 5 %} admin-list-inline{% endif %}"> | ||
| {% for website in total_websites %} | ||
| {% comment %}White space matters: do NOT reformat the following line{% endcomment %} | ||
| <li><a href="{{ website }}" target="_blank">{{ website }}</a></li> | ||
| {% endfor %} | ||
| </ul> | ||
| {% endif %} |
| {% if original_object.alternative_domains.all|length == 1 %} | ||
| <p class="margin-y-0 padding-y-0"> | ||
| <a href="{% url 'admin:registrar_website_change' original_object.alternative_domains.all.0.id %}?{{ 'return_path='|add:current_path }}" target="_blank">{{ original_object.alternative_domains.all.0 }}</a> | ||
| </p> | ||
| {% elif original_object.alternative_domains.all|length > 1 %} | ||
| <ul class="margin-top-0 margin-left-0 padding-left-0 admin-list-inline"> | ||
| {% for alt_domain in original_object.alternative_domains.all %} | ||
| {% comment %}White space matters: do NOT reformat the following line{% endcomment %} | ||
| <li><a href="{% url 'admin:registrar_website_change' alt_domain.id %}?{{ 'return_path='|add:current_path }}" target="_blank">{{alt_domain}}</a></li> | ||
| {% endfor %} | ||
| </ul> | ||
| {% endif %} |
There was a problem hiding this comment.
Yeah I see what you mean regarding the repetition. You made the right choice not to generalize it.
I think this is the "right way" to do things. I'm curious as to your thoughts: since it is the right way, do you think we should make it "known" in some way to other devs that we should follow this pattern in this case? Or does it not matter in your opinion?
There was a problem hiding this comment.
If you think it's worth it, we can discuss in eng huddle
There was a problem hiding this comment.
I don't think it'd hurt, but its not too important either. I can add an optional PSA about when to list a list and when not to
| normalized_expected = normalize_html(expected_markup) | ||
| normalized_response = normalize_html(response.content.decode("utf-8")) | ||
|
|
||
| index = normalized_response.find(normalized_expected) | ||
|
|
||
| # Assert that the expected markup is found in the response | ||
| if index == -1: | ||
| self.fail( | ||
| f"Expected markup not found in the response.\n\n" | ||
| f"Expected:\n{normalized_expected}\n\n" | ||
| f"Start index of mismatch: {index}\n\n" | ||
| f"Consider checking the surrounding response for context." | ||
| ) |
There was a problem hiding this comment.
I really like these tests. Nice work
Co-authored-by: zandercymatics <[email protected]>
|
🥳 Successfully deployed to developer sandbox el. |
|
🥳 Successfully deployed to developer sandbox el. |
Ticket
Resolves #2232
Changes
Context for reviewers
Let's talk about the normalize test helper method: This is a whitespace and line end stripper, and I'm using it on response.content which is a HUGE string. From running single tests, it does not seem like the computation time is significant which is good. When a test fails,
the assert vomits out the full 2 strings being asserted equal, which means that the console will get absolutely swamped if one of those tests failwe avoid doing a direct assert so's not to flood the console in case of failure, but rather do a manual substring test.I like the solid coverage the new tests give us,
but am wondering if we're better off doing multiple asserts that pieces of the expected content (single lines) are contained in the non-normalized response.Finally, the code repetition in the template. My gut feeling is if a block or pattern is repeated once, leave things alone. If it gets repeated thrice, pull things out into some common method/include etc.
Setup
Test alternative domains and websites when you have 1 item, less than 6 items, 6 or more items.
Code Review Verification Steps
As the original developer, I have
Satisfied acceptance criteria and met development standards
Ensured code standards are met (Original Developer)
Validated user-facing changes (if applicable)
As a code reviewer, I have
Reviewed, tested, and left feedback about the changes
Validated user-facing changes as a developer
Note: Multiple code reviewers can share the checklists above, a second reviewer should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A.
As a designer reviewer, I have
Verified that the changes match the design intention
Validated user-facing changes as a designer
References
Screenshots
1 website
Less than 6
6 or more
1 alt domain
More that 1 alt domains
